-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 remove deprecated workqueue metrics with invalid names #437
Conversation
Welcome @abursavich! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit around error messages, otherwise lgtm. Thanks!
pkg/metrics/workqueue.go
Outdated
ConstLabels: prometheus.Labels{"name": name}, | ||
}) | ||
if err := Registry.Register(depth); err != nil { | ||
log.Error(err, "failed to register "+metricName+" metric", "name", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error messages should never be variable -- that's what the key-value pairs are for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm happy to change them if you'd like, but the messages are actually constant expressions (metricName
is a local constant in each of the methods)
Thanks for the review, @DirectXMan12. PTAL. |
/lgtm |
The test failure was fixed with #466 ... I'll push again to retrigger Travis. |
After rebasing and picking up 17e0976, Github really doesn't want me to push https://github.com/kubernetes-sigs/controller-runtime/blob/master/.github/main.workflow to my repo.
|
I cherrypicked the race fix without the Github Action change, but, uhhh... I don't know how to get around this Github issue. If this PR goes through, I might try deleting my repo and re-forking post-Action commit. |
That's a bizarre message. Are you not using a normal git client? Keep me updated on that issue -- if it's causing you issues, it might cause others issues, so we may need to figure out a way around it or back it out. |
Migration work was already done to move controller-runtime from k8s v1.13 to v1.14, however this introduced certain issues. Unfortunately, workqueue_queue_duration_seconds and workqueue_work_duration_seconds were included in v0.1.10, but were recorded with microsecond values. Moving to k8s v1.14 corrects those values to seconds. Primarily this change removes the metrics that were deprecated in k8s v1.14, but were never previously included in v0.1 of controller-runtime. The one metric that was deprecated and is retained is longest_running_processor_microseconds, as it was included in v0.1.10.
It let me push it. I've rebased up to master, so this should work now. Will merge once tests pass. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abursavich, DirectXMan12 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Migration work was already done to move controller-runtime from k8s v1.13 to v1.14, however this introduced certain issues.
Unfortunately, workqueue_queue_duration_seconds and workqueue_work_duration_seconds were included in v0.1.10, but were recorded with microsecond values. Moving to k8s v1.14 corrects those values to seconds.
Primarily this change removes the metrics that were deprecated in k8s v1.14, but were never previously included in v0.1 of controller-runtime and thereby fixes #436.
The one metric that was deprecated and retained is longest_running_processor_microseconds, as it was included in v0.1.10.